I would say we shouldn't serialize both versions if we don't need to, but we expect compression to the common case, and the notCompressed serialization is presumably required to generate the compressed output, so this might be fine as-is. Still, if it isn't too difficult, it might be worth only serializing the one we need.

I agree that the serializeMessage function is a little confusing. It was already there so I haven't refactored it. It doesn't actually serialize both versions, it will only do the one that we have chosen and the other returned value will be an empty slice of bytes. I think that leads to confusion and this should be refactored but I have chosen to ignore it for now to not increase the scope of this change.

I'd be happy to refactor the function in this or a future PR though if you'd like? FYI from what I can see I don't think the compressed output requires anything from the notCompressed serialization to function, so it should be easy to split into two functions.

We should check that this message is the same as what we have in the buffer. If not, we need to clear the buffer (or preferably only clear this sequence number and anything after it), log an error saying that a reorg occurred, and then add this message.

Agreed, the backlogSegment.append method has an if statement with the following checks:

1. Is this message the next number in the sequence? +1 from the previous number
2. Else is this message larger than the expected next number in the sequence? If it is return errDropSegments error
3. Else this message must have already been seen and we return the errSequenceNumberSeen error

Let me know if you think this approach is missing something :)

There's an edge case here which is reorgs. When a reorg occurs, a message can be replaced by a different message with the same sequence number. However, I'm beginning to think that what we should do in reorg cases is too ambiguous and error prone, and we should just rely on the L1 sequencer inbox to sort it out and establish a canonical message ordering, and then pick up the feed from there. I.e. the current approach should be fine. I'll probably discuss this with the nitro team and make sure this approach makes sense to everyone.

Btw, this log.Info line needs to use the key-value logging scheme instead of %s.

I'm not sure this quite works because head was already loaded before in b.head.Start(). Maybe you could reload head after the Lookup call?

Good shout, changed

We need to be careful about recursive read locks. They cause deadlocks:

If a goroutine holds a RWMutex for reading and another goroutine might call Lock, no goroutine should expect to be able to acquire a read lock until the initial read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock.

https://pkg.go.dev/sync#RWMutex.RLock

I'd suggest having a lowercase start() function which doesn't lock the mutex and is used by both Start() and this function.

Good catch! I have changed it over

I doubt this would matter too much but ideally we'd create tmp to only be the size of endIndex - startIndex and copy that section

I actually tried to do that with:

tmp := make([]*m.BroadcastFeedMessage, endIndex-startIndex)
copy(tmp, s.messages[startIndex:endIndex])
return tmp, nil

but for some reason it seemed to copy messages from the s.messages slice outside of the startIndex to endIndex range that I had specified. I'm not sure why and maybe I did something wrong. For now I will leave this as is as it fixed the errors I saw in the test.

actually I must have tried something else when copying just that section of the slice as I added it and it is passing the tests. I think earlier I might have used a different length than endIndex-startIndex when making the tmp slice

Same thing in this function about recursive locking

Good catch! I have changed it over

I think there's some calls to End that have the same issue